Skip to content

Fixed LMDB collection errors #1787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fixed LMDB collection errors #1787

wants to merge 1 commit into from

Conversation

airween
Copy link
Member

@airween airween commented May 28, 2018

When I compiled ModSecurity with LMDB support, the related regression tests had failed, eg:

72 collection-resource-simple.json        Testing collection :: RESOURCE (2/2)           failed!
73 collection-resource.json               Testing collection :: RESOURCE (1/2)           failed!
74 collection-resource.json               Testing collection :: RESOURCE (2/2)           failed!

Looks like the LMDB backend isn't complete, but may be I'm wrong :).

Before you merge it, please review the comments here:
https://github.com/airween/ModSecurity/blob/908782a06af9d26962e244745bfa46d659d91828/src/collection/backend/lmdb.cc#L496
https://github.com/airween/ModSecurity/blob/908782a06af9d26962e244745bfa46d659d91828/src/collection/backend/lmdb.cc#L509
https://github.com/airween/ModSecurity/blob/908782a06af9d26962e244745bfa46d659d91828/src/collection/backend/lmdb.cc#L518

I don't understand some things, please help me to clarify them.

The patch fixed the regression tests without touched them.

zimmerle pushed a commit that referenced this pull request Jun 12, 2018
Improvements on top of #1787
e51297b
@zimmerle
Copy link
Contributor

Hi @airween,

Indeed those are good questions. I've changed the implementation a little bit to address some of your concerns:

Most of the checks that you have highlighted was there due to historical reasons. In the past the collection key was named after a concatenation of the collection name and key (+ others factors such as the web app id, not so relevant for the explanation).
The fact is that naming police was changed for performance improvements and the changes were not back ported to the LMDB back-end.

I've changed yours implementation to reflect the changes on InMemoryPerProcess collection. The modifications are available here: e51297b.

If you are OK with the patch above, I will proceed with the merge. Thank you.

@zimmerle zimmerle self-assigned this Jun 12, 2018
@zimmerle zimmerle added the 3.x Related to ModSecurity version 3.x label Jun 12, 2018
@zimmerle zimmerle added this to the v3.0.3 milestone Jun 12, 2018
@airween
Copy link
Member Author

airween commented Jun 12, 2018

Hi Felipe,

I've grabbed the pull_1787 branch, merged to my lmdbfix, reviewed the modifications and tests. All tests had passed. The modifications looks like right:

https://github.com/SpiderLabs/ModSecurity/compare/v3/master...airween:v3/lmdbfix_1787?expand=1

I think you can merge it to v3/master. Thanks.

@airween airween closed this Jun 12, 2018
@airween airween reopened this Jun 12, 2018
zimmerle pushed a commit that referenced this pull request Jun 12, 2018
CHANGES: Adds info about: #1787
81708a6
@zimmerle
Copy link
Contributor

Merged! Thanks!

@zimmerle zimmerle closed this Jun 12, 2018
@airween airween deleted the v3/lmdbfix branch June 13, 2018 08:59
@victorhora victorhora mentioned this pull request Nov 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@zimmerle zimmerle

Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Milestone
v3.0.3
Development

Successfully merging this pull request may close these issues.
2 participants
@airween @zimmerle